Prebuilt GHCR image for E2E CI - #33
Conversation
c2c24b0 to
d016f7b
Compare
d016f7b to
bd2c622
Compare
📝 WalkthroughWalkthroughAdds deterministic GHCR image tags, a conditional GPU image publishing workflow, build-only Docker support, and CI steps that pull matching images or fall back to local builds. ChangesPrebuilt image CI
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant GHCR
participant Docker
CI->>GHCR: Authenticate and inspect image tag
alt Image exists
GHCR-->>CI: Return existing manifest
else Image is missing
CI->>Docker: Build image in build-only mode
Docker-->>CI: Return local image
CI->>GHCR: Push computed tag
end
CI->>GHCR: Pull image for GPU E2E
GHCR-->>CI: Return matching image
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
bd2c622 to
c01ef9f
Compare
bb2525a to
3b60fbe
Compare
c01ef9f to
4a71524
Compare
3b60fbe to
8ab287d
Compare
1ad2d8d to
3c68232
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-image.yml:
- Around line 12-15: Update the workflow trigger configuration under the
top-level on key to use the compact [main] branch list instead of [ "main" ]. If
YAML lint still flags on as a truthy value, quote that key or apply the
repository’s established yamllint configuration.
- Around line 29-31: Move the contents and packages permission declarations from
workflow scope into the build_and_push job’s permissions block, preserving
contents: read and packages: write while limiting GHCR write access to that
publishing job.
In @.github/workflows/ci.yml:
- Around line 108-109: Update the workflow step invoking pull_or_build_image.sh
so GHCR authentication and image pulling are best-effort: prevent docker login
failures from terminating the job, and ensure any login or pull failure reaches
the script’s local-build fallback rather than exiting the workflow.
- Line 20: Move the packages: read permission from the workflow-wide permissions
block into the test_e2e job, adding an explanatory comment that only this job
pulls from GHCR. Ensure unrelated jobs such as pre_commit no longer receive
package-read authority.
In `@scripts/ci/pull_or_build_image.sh`:
- Around line 22-27: Update the docker pull failure branch in the image handling
flow to remove the existing local tag before announcing the local-build
fallback. Use the same LOCAL_TAG referenced by the successful docker tag and
ensure cleanup does not prevent run_docker.sh from proceeding to build locally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a1921606-a318-4787-840a-98e0be506a0c
📒 Files selected for processing (6)
.github/workflows/build-image.yml.github/workflows/ci.ymlREADME.mddocker/run_docker.shscripts/ci/image_tag.shscripts/ci/pull_or_build_image.sh
| on: | ||
| workflow_dispatch: | ||
| push: | ||
| branches: [ "main" ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported YAML lint error.
Use [main] instead of [ "main" ]; if the truthy-value rule is enforced, quote the on key or adjust the yamllint configuration.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 12-12: truthy value should be one of [false, true]
(truthy)
[error] 15-15: too many spaces inside brackets
(brackets)
[error] 15-15: too many spaces inside brackets
(brackets)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-image.yml around lines 12 - 15, Update the workflow
trigger configuration under the top-level on key to use the compact [main]
branch list instead of [ "main" ]. If YAML lint still flags on as a truthy
value, quote that key or apply the repository’s established yamllint
configuration.
Source: Linters/SAST tools
| permissions: | ||
| contents: read | ||
| packages: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope GHCR write access to the publishing job.
Move contents: read and packages: write under jobs.build_and_push.permissions. Keeping package write at workflow scope unnecessarily grants it to any future job.
🔐 Proposed fix
-permissions:
- contents: read
- packages: write
-
jobs:
build_and_push:
+ permissions:
+ contents: read
+ packages: write📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: | |
| contents: read | |
| packages: write | |
| jobs: | |
| build_and_push: | |
| permissions: | |
| contents: read | |
| packages: write |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 31-31: overly broad permissions (excessive-permissions): packages: write is overly broad at the workflow level
(excessive-permissions)
[warning] 31-31: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-image.yml around lines 29 - 31, Move the contents
and packages permission declarations from workflow scope into the build_and_push
job’s permissions block, preserving contents: read and packages: write while
limiting GHCR write access to that publishing job.
Source: Linters/SAST tools
|
|
||
| permissions: | ||
| contents: read | ||
| packages: read |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope packages: read to test_e2e.
This permission is currently workflow-wide, so unrelated jobs such as pre_commit receive package-read authority. Move it to the E2E job with an explanatory comment; only that job pulls from GHCR.
Proposed permission scope
permissions:
contents: read
- packages: read
jobs:
test_e2e:
+ permissions:
+ contents: read
+ # Required to pull the prebuilt image from GHCR.
+ packages: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| packages: read | |
| permissions: | |
| contents: read | |
| jobs: | |
| test_e2e: | |
| permissions: | |
| contents: read | |
| # Required to pull the prebuilt image from GHCR. | |
| packages: read |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 20-20: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 20, Move the packages: read permission from
the workflow-wide permissions block into the test_e2e job, adding an explanatory
comment that only this job pulls from GHCR. Ensure unrelated jobs such as
pre_commit no longer receive package-read authority.
Source: Linters/SAST tools
| - name: Pull prebuilt image (fast path) | ||
| run: ./scripts/ci/pull_or_build_image.sh |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make GHCR authentication best-effort.
A failed docker login exits the job before pull_or_build_image.sh can fall back to a local build. Make authentication non-blocking, or move it into the script’s best-effort path, and ensure login or pull failures invoke the local build.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 108 - 109, Update the workflow step
invoking pull_or_build_image.sh so GHCR authentication and image pulling are
best-effort: prevent docker login failures from terminating the job, and ensure
any login or pull failure reaches the script’s local-build fallback rather than
exiting the workflow.
| if docker pull "${REMOTE}"; then | ||
| docker tag "${REMOTE}" "${LOCAL_TAG}" | ||
| echo ">>> Tagged as ${LOCAL_TAG}; run_docker.sh will reuse it (no build)." | ||
| else | ||
| echo ">>> Prebuilt image unavailable (cache miss); run_docker.sh will build locally." | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate stale local images on pull failure.
If docker pull fails, an older isaac_autodata:curobo tag remains available. run_docker.sh can then reuse that stale image instead of taking the documented local-build fallback.
🐛 Proposed fix
else
+ docker image rm -f "${LOCAL_TAG}" >/dev/null 2>&1 || true
echo ">>> Prebuilt image unavailable (cache miss); run_docker.sh will build locally."
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if docker pull "${REMOTE}"; then | |
| docker tag "${REMOTE}" "${LOCAL_TAG}" | |
| echo ">>> Tagged as ${LOCAL_TAG}; run_docker.sh will reuse it (no build)." | |
| else | |
| echo ">>> Prebuilt image unavailable (cache miss); run_docker.sh will build locally." | |
| fi | |
| if docker pull "${REMOTE}"; then | |
| docker tag "${REMOTE}" "${LOCAL_TAG}" | |
| echo ">>> Tagged as ${LOCAL_TAG}; run_docker.sh will reuse it (no build)." | |
| else | |
| docker image rm -f "${LOCAL_TAG}" >/dev/null 2>&1 || true | |
| echo ">>> Prebuilt image unavailable (cache miss); run_docker.sh will build locally." | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ci/pull_or_build_image.sh` around lines 22 - 27, Update the docker
pull failure branch in the image handling flow to remove the existing local tag
before announcing the local-build fallback. Use the same LOCAL_TAG referenced by
the successful docker tag and ensure cleanup does not prevent run_docker.sh from
proceeding to build locally.
…t path - build-image.yml builds the cuRobo image and pushes it to GHCR, keyed by a content hash of the image inputs (scripts/ci/image_tag.sh) - scripts/ci/pull_or_build_image.sh pulls that prebuilt image when available and falls back to a local build on a cache miss; ci.yml uses it before the E2E run - run_docker.sh gains a -b build-only flag so the image can be built without launching a container
3c68232 to
a3107ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-image.yml:
- Around line 69-72: Add an “Log out of GHCR” step immediately after the image
push flow, configured with if: always(), that runs docker logout ghcr.io and
tolerates logout failure so credentials are removed on every job exit.
- Around line 84-86: Update the “Build image” workflow step to pass the
force-rebuild option to docker/run_docker.sh, ensuring any existing
isaac_autodata:curobo image is rebuilt before publishing while preserving the
current conditional execution.
- Around line 38-42: Use a single CUDA architecture value for both image tagging
and building: define IMAGE_CUDA_ARCH explicitly in the publishing and E2E jobs,
derive or align TORCH_CUDA_ARCH_LIST with it, and add validation that the two
values match before running either workflow.
In @.github/workflows/ci.yml:
- Around line 103-106: Add a GHCR logout cleanup step after the E2E tests and
login flow, using the workflow’s cleanup step with if: always() and ensuring
docker logout ghcr.io does not fail the job.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0ba7eb50-7fe9-4a90-af5d-b70b94fd0339
📒 Files selected for processing (6)
.github/workflows/build-image.yml.github/workflows/ci.ymlREADME.mddocker/run_docker.shscripts/ci/image_tag.shscripts/ci/pull_or_build_image.sh
| # cuRobo arch baked into the image. Keep in sync with IMAGE_CUDA_ARCH in | ||
| # scripts/ci/image_tag.sh so the published tag matches what consumers pull. | ||
| env: | ||
| TORCH_CUDA_ARCH_LIST: "8.9+PTX" | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Enforce one CUDA-architecture input across tagging and building.
image_tag.sh hashes IMAGE_CUDA_ARCH, while this workflow independently sets TORCH_CUDA_ARCH_LIST. If they drift, an image can be published under a tag computed for a different architecture. Set IMAGE_CUDA_ARCH explicitly in both publishing and E2E jobs and validate that both values match.
Proposed guard
env:
+ IMAGE_CUDA_ARCH: "8.9+PTX"
TORCH_CUDA_ARCH_LIST: "8.9+PTX"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-image.yml around lines 38 - 42, Use a single CUDA
architecture value for both image tagging and building: define IMAGE_CUDA_ARCH
explicitly in the publishing and E2E jobs, derive or align TORCH_CUDA_ARCH_LIST
with it, and add validation that the two values match before running either
workflow.
| - name: Log in to GHCR | ||
| env: | ||
| GHCR_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: echo "$GHCR_TOKEN" | docker login ghcr.io -u "${{ github.actor }}" --password-stdin |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Log out of GHCR on every job exit.
This self-hosted runner is reusable, and docker login leaves credentials in Docker’s configuration. Add an if: always() cleanup step after the push.
Proposed cleanup
- name: Log out of GHCR
if: always()
run: docker logout ghcr.io || true🧰 Tools
🪛 zizmor (1.26.1)
[error] 72-72: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-image.yml around lines 69 - 72, Add an “Log out of
GHCR” step immediately after the image push flow, configured with if: always(),
that runs docker logout ghcr.io and tolerates logout failure so credentials are
removed on every job exit.
| - name: Build image | ||
| if: steps.exists.outputs.build == 'true' | ||
| run: ./docker/run_docker.sh -c -b |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Force a fresh build before publishing.
Because this runs on a persistent self-hosted runner, isaac_autodata:curobo may already exist locally. Without -r, the workflow can reuse that stale image and publish it under the new content-hash tag.
Proposed fix
- run: ./docker/run_docker.sh -c -b
+ run: ./docker/run_docker.sh -c -r -b📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build image | |
| if: steps.exists.outputs.build == 'true' | |
| run: ./docker/run_docker.sh -c -b | |
| - name: Build image | |
| if: steps.exists.outputs.build == 'true' | |
| run: ./docker/run_docker.sh -c -r -b |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-image.yml around lines 84 - 86, Update the “Build
image” workflow step to pass the force-rebuild option to docker/run_docker.sh,
ensuring any existing isaac_autodata:curobo image is rebuilt before publishing
while preserving the current conditional execution.
| - name: Log in to GHCR | ||
| env: | ||
| GHCR_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: echo "$GHCR_TOKEN" | docker login ghcr.io -u "${{ github.actor }}" --password-stdin |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Log out of GHCR after the E2E job.
The self-hosted runner is reused, and docker login leaves credentials in Docker’s configuration. Add an if: always() cleanup step after the tests.
Proposed cleanup
- name: Log out of GHCR
if: always()
run: docker logout ghcr.io || true🧰 Tools
🪛 zizmor (1.26.1)
[error] 106-106: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 103 - 106, Add a GHCR logout cleanup
step after the E2E tests and login flow, using the workflow’s cleanup step with
if: always() and ensuring docker logout ghcr.io does not fail the job.
1 participant
Summary
Cuts the E2E job's cold-start cost. Every premerge run currently rebuilds the
isaac_autodata:curoboimage from scratch (~29 min, dominated by the cuRobo compile) because the runner prunes Docker between jobs. This publishes the image to GHCR and pulls it when the inputs are unchanged.build-image.ymlbuilds the image and pushes it to GHCR, tagged by a content hash of the image inputs (scripts/ci/image_tag.sh:docker/,.gitmodules, theIsaacLab-Arenagitlink,setup.py,pyproject.toml, and the CUDA arch).scripts/ci/pull_or_build_image.shpulls that prebuilt image when the hash matches and falls back to a local build on a cache miss;ci.ymlruns it before the E2E tests.run_docker.shgains a-bbuild-only flag so the image can be built without launching a container.Based on
main(the base CI landed in #25). The GHCR speedup takes effect oncebuild-image.ymlpublishes an image frommainafter this merges; until then the premerge job falls back to a local build (no regression).Test plan
pre_commitgreentest_e2egreen (fell back to local build this run; prebuilt pull activates post-merge)build-image.ymlpublishes to GHCR and a subsequent premerge run pulls it (fast path)Summary by CodeRabbit
New Features
Documentation